Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add non-copying version of murmur3_128 that reads directly from a byte buffer #23

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Add non-copying version of murmur3_128 that reads directly from a byte buffer #23

merged 4 commits into from
Sep 19, 2024

Conversation

ShiKaiWi
Copy link
Contributor

@ShiKaiWi ShiKaiWi commented Sep 9, 2024 •
edited
Loading

Close #20

Refering to the implementation of murmur3_32_of_slice, provide murmur3_x64_128_of_slice and murmur3_x86_128_of_slice.

And the benchmark shows the improvement:

test bench_32            ... bench:          69.78 ns/iter (+/- 0.65) = 811 MB/s
test bench_32_slice      ... bench:           5.28 ns/iter (+/- 0.05) = 11200 MB/s
test bench_c_32          ... bench:          17.34 ns/iter (+/- 0.15) = 3294 MB/s
test bench_c_x64_128     ... bench:          10.94 ns/iter (+/- 0.11) = 5600 MB/s
test bench_c_x86_128     ... bench:          19.61 ns/iter (+/- 0.33) = 2947 MB/s
test bench_x64_128       ... bench:          24.23 ns/iter (+/- 0.35) = 2333 MB/s
test bench_x64_128_slice ... bench:           9.78 ns/iter (+/- 0.12) = 6222 MB/s
test bench_x86_128       ... bench:          27.90 ns/iter (+/- 0.57) = 2074 MB/s
test bench_x86_128_slice ... bench:          16.11 ns/iter (+/- 0.73) = 3500 MB/s

@ShiKaiWi
Copy link
Contributor Author

ShiKaiWi commented Sep 9, 2024

@stusmall Hi, would you please take a look at this changeset?

stusmall
stusmall requested changes Sep 10, 2024
View reviewed changes
src/murmur3_x64_128.rs Outdated
/// use murmur3::murmur3_x64_128_of_slice;
/// let hash_result = murmur3_x64_128_of_slice(b"hello world", 0);
/// ```
pub fn murmur3_x64_128_of_slice(source: &[u8], seed: u32) -> Result<u128> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is returning a Result, but I believe the operation is infallable. Is that correct? Converting this to result u128 would probably be my only change

Copy link
Contributor Author

@ShiKaiWi ShiKaiWi Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stusmall Thanks for your review, which makes sense. I've addressed it. PTAL again. ❤️

@ShiKaiWi
Copy link
Contributor Author

@stusmall Hi, the requested changes have been finished. Would you please take a look again?

@stusmall
Copy link
Owner

Sorry about the slow reply. Work has been chaotic and I wanted to set aside to give a more careful read. This looks great

I'm going to do some changes in the coming days to upgrade deps and add no_std support. I'll have that go out in that version

@stusmall stusmall merged commit 2c39087 into stusmall:master Sep 19, 2024
4 checks passed
@ShiKaiWi ShiKaiWi deleted the feat-hash-128b-of-slice branch September 20, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stusmall stusmall stusmall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

non-copying (*_of_slice) variants of 128 bit murmur hashes
2 participants
@ShiKaiWi @stusmall